Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ST_QuantizeCoordinates(): speed-up implementation #718

Merged
merged 1 commit into from
Jan 1, 2023

Conversation

rouault
Copy link
Contributor

@rouault rouault commented Dec 27, 2022

We can completely avoid any floating-point computation by directly using the fact that the IEEE-754 representation of a floating point number contains the integral part of log2(abs(number)).
The improved trim_preserve_decimal_digits() function is ~ 7x times faster.

@Komzpa
Copy link
Member

Komzpa commented Dec 27, 2022

clang is not happy about it:

Died at ./regress/run_test.pl line 796.
 ./regress/core/quantize_coordinates .. failed (psql exited with an error: /tmp/pgis_reg/test_45_out)
-----------------------------------------------------------------------------
t1|t
psql:quantize_coordinates.sql:4: ERROR:  Must specify precision
t3|t
t4|t
t5|t
psql:quantize_coordinates.sql:14: server closed the connection unexpectedly
make: *** [regress/runtest.mk:24: check-regress] Error 2
	This probably means the server terminated abnormally
	before or while processing the request.
psql:quantize_coordinates.sql:14: error: connection to server was lost
-----------------------------------------------------------------------------

We can completely avoid any floating-point computation by directly using
the fact that the IEEE-754 representation of a floating point number
contains the integral part of log2(abs(number)).
The improved trim_preserve_decimal_digits() function is ~ 7x times
faster.
@rouault
Copy link
Contributor Author

rouault commented Dec 27, 2022

clang is not happy about it:

ok, issue solved by adding a int cast in the expression (int)((dint >> 52) & 2047) - 1023; otherwise I suspect the sanitizer flag would complain about unsigned integer underflow when computing negative exponents.

@robe2
Copy link
Member

robe2 commented Jan 1, 2023

@Komzpa @pramsey @rouault What's your thought about backporting to 3.3 as well. Seems change is harmless enough. I'll push to 3.4 (master) once the PG16 new GHA docker build is done.

@rouault
Copy link
Contributor Author

rouault commented Jan 1, 2023

Seems change is harmless enough.

Personnally I wouldn't backport this. This is really in the enhancement category, and it might change a bit results. I believe the new implementation is less pessimistic than the previous one (that is to say the number of nullified bits can be a bit higher, while meeting the expressed precision), so this will result in slightly different results.

@strk strk closed this in e5947ca Jan 1, 2023
@strk strk merged commit bef8e1c into postgis:master Jan 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants